[MNT] moving linting to ruff, adding editorconfig#653
[MNT] moving linting to ruff, adding editorconfig#653fkiraly merged 33 commits intoPyPortfolio:mainfrom
ruff, adding editorconfig#653Conversation
|
@fkiraly I install an .editorconfig file and also deactivate the linting business (which will later be replaced with ruff) |
|
@fkiraly this could be merged |
There was a problem hiding this comment.
Great! Though I would not sequence it to include a full rework of linting but do it in one PR.
- I agree with moving to
ruff! - I think a single PR should replace current config with
ruff, so there is no intermediate state where linting is off - the codecov change seems unrelated, should it be in this PR?
Otherwise, agreed with the direction, except that linting upgrade should happen in the same PR.
Generally, I would kindly like to request descriptive PR titles and descriptions of what a PR is doing. If you accidentally opened with an empty description, you can edit it (dots at the top right).
fkiraly
added
the
maintenance
fkiraly
changed the title
ruff, adding editorconfig
|
uvx pre-commit run --show-diff-on-failure --color=always --all-files |
|
@fkiraly This is now using ruff. For now, I have deactivated most of my usual rules -- to avoid the code blood bath. You can execute ruff locally using "uvx ruff check ." I have also added ruff as a pre-commit hook. |
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Great!
Instead of ruff.toml, can we move to pyproject.toml to have all configs in a single place?
We should also make sure that current linting is mapped to ruff
|
I am not sure it's a great idea to move ruff.toml in its entity to pyproject.toml. I think no linting should be in pyproject.toml |
|
I remember why I like ruff.toml. It's a lot of material and usually it's very project independent. So you can copy it directly from a template. If it's integrated into pyproject it's harder |
| @@ -1,388 +1,391 @@ | |||
| { | |||
| "cells": [ | |||
There was a problem hiding this comment.
can we avoid linting settings that change the indentation in the notebooks?
There was a problem hiding this comment.
isort messed with the notebook files. I recommend excluding the notebook files (from linting) for now
|
@fkiraly I have integrated ruff.toml into pyproject. Checked locally with "uvx ruff check ." still works. Excluded also the notebooks from listing (from now on...) |
|
@fkiraly, please merge but without the notebooks if possible |
|
@fkiraly please merge this before you change the notebooks again in the test notebooks request. |
well, did not see this message. The best practice to advertise sth like this is in PR2, you (a) include a description at the top, and (b) a line like "depends on #653 which should be merged first". Information on merge order desired should always be in the top descr. Either way, what do we do now about the notebooks? Does not look like there is a merge conflict? |
There was a problem hiding this comment.
Great PR!
Made some small changes:
- removed pre-commit steps that are based on packages from small providers. This is to reduce the surface for security risks, we had supply chain attacks where small actions packages got attacked before.
- moved the pre-commit job into
main, and make it a gate for the other jobs, to ensure we do not run CI unless the files are linted. - applies linting only to changed files, or the job currently fails.
2 participants
ruffeditorconfigAlso fixes linting in some files (but not all).